Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add flake for supporting NixOs and nix systems #110

Merged
merged 2 commits into from
May 30, 2024

Conversation

brumik
Copy link
Contributor

@brumik brumik commented May 25, 2024

This PR adds Nix and NixOs support for amber. It packages the executable with bc and bash.

In addition it fixes the /bin/bash executable path since it is not supported on all systems (for example nix) replacing it with the truly universal /usr/bin/env bash.

The readme is updated to reflect NixOs possible install process (this is highly individual as nix is doing source based package management and all you need is the structure of the flake.nix).

shell.nix and default.nix are there for the nix-shell and nix-build (in case not using flakes). .envrc is allowing the use of direnv for easy development setup on nix.

Closes #109

@brumik
Copy link
Contributor Author

brumik commented May 25, 2024

I am not sure what is the testing process if any, but let me know. Also I had no chance to try out the new bash path in any other systems than ubuntu and nix, so let me know if there is any problem with it in any other systems.

@Ph0enixKM Ph0enixKM self-requested a review May 25, 2024 19:08
Ph0enixKM
Ph0enixKM previously approved these changes May 25, 2024
Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the new bash path works. Interesting to see NixOS support! I've no way to test this but I trust you that it works. I'll add the README part to documentation once this gets merged

@Ph0enixKM Ph0enixKM linked an issue May 25, 2024 that may be closed by this pull request
@brumik
Copy link
Contributor Author

brumik commented May 25, 2024

Great, what are the next steps?

(I also highly recommend trying out NixOs and nixpgs in general, pretty cool to do development on especially for command line)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@b1ek b1ek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

im not sure that its necessary to pollute repo's root with files. can you create a directory structure like this?

(root)
|- packages
|--- nix
|----- default.nix
|----- ...

also is it really necessary to host this kind of thing on a main repo? don't packages usually have their own?

@uncenter
Copy link
Contributor

im not sure that its necessary to pollute repo's root with files. can you create a directory structure like this?

(root)
|- packages
|--- nix
|----- default.nix
|----- ...

also is it really necessary to host this kind of thing on a main repo? don't packages usually have their own?

Typically the <root>/nix/ directory is used.

@brumik
Copy link
Contributor Author

brumik commented May 26, 2024

@uncenter It is possible with appending ?dir=nix in the url command, though I never seen it used, could you provide some well known examples? As far as I know this is not default behaviour.

@b1ek The nixpkgs can wrap your release with nix for sure, but it is not preferred (you need to have a maintainer updating versions and dependencies for this repo in a third party repo). This way it is also easy and unified to set up development env for Amber. (If you do not include it everybody on nix will have to write it himself, gitignore it before able to work on the project).

That's said these files are in root because they are just like .nmprc Dockerfile build.sh or project.toml. They contain packaging information. You can of course put these files in sub-folders and modify all your commands and paths to account for not being in the root even though it is not the default expected behavior.

@Ph0enixKM
Copy link
Member

Ph0enixKM commented May 26, 2024

@brumik does this installer build the Amber from scratch or does it use the latest release on GitHub? I'm trying to figure out how to solve it. By no means I've never used NixOS but by searching things on the web I've seen some people do some custom Bash in the installPhase of the flake config file. Let me know what if there is something I can help you with

@brumik
Copy link
Contributor Author

brumik commented May 26, 2024

It is not bash, but sh. Bash is not the default. You can of course run commands when building the package but in this case it is not needed. That's more of a case when you wrap an existing project and you need to do some more advanced manipulations with the compile targets.

It is hard to wrap your head around Nix but Nix is Source based and not Package based 'distro'. This means that the user compiles the program on their machine from a public repo URL. So in the example in readme it is using the main branch. But you can also specify a release branch or release tag or just a plain commit SHA.

As this project is not Nix fist project I would expect having Tags for each release that people on nix could reference.

(There are of course compiled binaries on a cache server for nixpkgs since you don't want to compile chrome and such on your machine, but that's catching detail and not how nix works in the first place.)

@b1ek
Copy link
Member

b1ek commented May 26, 2024

That's said these files are in root because they are just like .nmprc Dockerfile build.sh or project.toml.
...
Nix is Source based and not Package based 'distro'

The problem is, if everything gets thrown into repo's root it will make a horrible mess out of it. By "everything", i mean that Nix would not be the first distro to host package configs in the root of the repo.

I mean, just a few files is not a problem, but unspeakable horrors will unfold when 10 more distros or packaging systems like Nix will put their files in the root.

What about creating a separate repo only with config files and having github actions fetch the source & config files for each release/commit? That way, its easier to file issues and/or PRs about the package, the main repo is clean and nice and Nix packaging system is happy

@brumik
Copy link
Contributor Author

brumik commented May 26, 2024

There is no GitHub action that needs to be run. Flake.nix is telling NixOS what is needed to compile the code and how to compile it. So putting in new repo makes sense if the source code is in the new repo (aka a fork with one extra commit that needs to be synced)

@brumik
Copy link
Contributor Author

brumik commented May 26, 2024

We can probably remove default.nix and shell.nix since it is purely development purposes if we ship with flakes.

A example repo with super similar setup (CMD line tool written in rust) is https://github.com/jrmoulton/tmux-sessionizer

@Ph0enixKM
Copy link
Member

@brumik will this package require future maintenance or we can just leave this flake file and call it a day?

Does the installer recompile from source and update versions and tags on each install?

@uncenter
Copy link
Contributor

@brumik will this package require future maintenance or we can just leave this flake file and call it a day?

For the most part, no. Some rare cases where updating might be needed.

Does the installer recompile from source and update versions and tags on each install?

This flake instructs Nix how to build it from source, so yeah "recompile from source" is true but I'm not sure what you mean "update versions and tags on each install".

@Ph0enixKM
Copy link
Member

@uncenter I meant that it updates the Amber version automatically in the nix pkg registry when it detects that the Cargo.toml version changed. But I assume it does.

@uncenter
Copy link
Contributor

@uncenter I meant that it updates the Amber version automatically in the nix pkg registry when it detects that the Cargo.toml version changed. But I assume it does.

Currently there are two versions of the Amber package - one is upstream in https://github.com/NixOS/nixpkgs, which is auto-updated by a bot that detects when you release a new version (I am a maintainer for this package FYI), and the other is this new flake in this repository. This flake will always be fully up to date.

.envrc Outdated Show resolved Hide resolved
src/compiler.rs Show resolved Hide resolved
default.nix Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
Comment on lines +3 to +5
naersk.url = "github:nix-community/naersk/master";
nixpkgs.url = "github:NixOS/nixpkgs/nixpkgs-unstable";
utils.url = "github:numtide/flake-utils";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see much point in adding flake-utils or naersk. But it's fine, we can certainly add them.

flake.nix Outdated Show resolved Hide resolved

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we could add an overlay above the nixpkgs package amber-lang

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@isabelroses
Copy link

Hi, sorry to absolutely drop a massive review, but I worked with the team packaging this for upstream nixpkgs, and I would prefer it was done correctly here to. Thanks for doing this though.

@brumik
Copy link
Contributor Author

brumik commented May 27, 2024

@isabelroses It is much appreciated, great place to learn more.

@isabelroses
Copy link

@uncenter It is possible with appending ?dir=nix in the url command, though I never seen it used, could you provide some well known examples? As far as I know this is not default behaviour.

They did this with neovim before they moved the flake off the main repo ?dir=contrib though.

@Ph0enixKM
Copy link
Member

Thanks @isabelroses for your feedback!

@brumik
Copy link
Contributor Author

brumik commented May 28, 2024

I updated the PR and removed the default.nix and shell.nix files. @isabelroses and @b1ek could you give it one more look?

flake.nix Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
Copy link

@isabelroses isabelroses left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builds and runs correctly

README.md Outdated Show resolved Hide resolved
Copy link
Member

@b1ek b1ek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, but the readme is kind of a mess

README.md Outdated

The package is already containing all the required install scripts and dependencies. You can use the flake as:

```nix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is all that info really necessary? if thats just a package, you should mention it with one line under the "Via package manager" header:

Nix - amber-lang

if you really need this info, you could wrap it in a dropdown box so it doesnt take up much space

like this
<details>
<summary>Title</summary>

### Put stuff here
</details>

im just saying that not every amber user is a nix user. they don't need this information.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also nothing is preventing you from creating a NIX.md file near README.md with the necessary instructions

README.md Outdated
@@ -37,6 +38,26 @@ Amber is packaged in the following distros:

Arch (AUR) - `amber-bash-bin`

#### Nix

The package is already containing all the required install scripts and dependencies. You can use the flake as:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grammar: package is not "is already containing", it "contains"

@Ph0enixKM
Copy link
Member

Yeah @brumik please move the installation details to NIX.md as it's a whole subsection that I'll add in the Documentation itself. Please mention it in the original README.md including the hyperlink to NIX.md. I think this PR is almost finalized and we'll be merging it.

@Ph0enixKM Ph0enixKM dismissed their stale review May 29, 2024 10:58

I'll wait for the changes

Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@b1ek b1ek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@b1ek b1ek merged commit d0d8fde into amber-lang:master May 30, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

✨ [Improvement] don't hardcode path to bash
5 participants